Conversation
dg0yt
left a comment
dg0yt
left a comment
There was a problem hiding this comment.
Nitpick: When updating the patch, try to avoid unnecessary whitespace changes (removing/moving empty lines).
| # Add the library to the project | ||
| -add_library(wincpp STATIC) | ||
| +add_library(wincpp) | ||
| +set_target_properties(wincpp PROPERTIES WINDOWS_EXPORT_ALL_SYMBOLS ON) |
There was a problem hiding this comment.
Reminder, this is not accepted in vcpkg.
https://learn.microsoft.com/en-us/vcpkg/contributing/maintainer-guide#do-not-add-cmake_windows_export_all_symbols
(That CMake variable determines the propery.)
There was a problem hiding this comment.
Note that you can use vcpkg_check_linkage(ONLY_STATIC_LIBRARY)
| +set(wincpp_CONFFILE_DEST "${CMAKE_INSTALL_DATAROOTDIR}/wincpp") | ||
| + | ||
| +install(FILES | ||
| + "${CMAKE_CURRENT_BINARY_DIR}/wincppConfig.cmake" |
There was a problem hiding this comment.
Reminder, the "unofficial" prefix must be used for config and targets unless provided officially by upstream.
| -DBUILD_EXAMPLES=OFF | ||
| -DBUILD_PACKAGE=OFF |
There was a problem hiding this comment.
| -DBUILD_EXAMPLES=OFF | |
| -DBUILD_PACKAGE=OFF | |
| -DBUILD_EXAMPLES=OFF | |
| -DBUILD_PACKAGE=OFF |
| -target_sources(_wincpp_core INTERFACE ${header_files}) | ||
| +target_sources(_wincpp_core INTERFACE $<BUILD_INTERFACE:${header_files}>) |
There was a problem hiding this comment.
Hm, what did upstream want to achieve? Is it still achieved after this change? Do we need a test port?
There was a problem hiding this comment.
To the best of my understanding this doesn't actually do anything? INTERFACE says only applies to linkers but BUILD_INTERFACE says only applies during build...
BillyONeal
left a comment
BillyONeal
left a comment
There was a problem hiding this comment.
I can't merge WINDOWS_EXPORT_ALL_SYMBOLS, sorry :(
wincpp provides CMake targets:
# this is heuristically generated, and may not be correct
find_package(wincpp CONFIG REQUIRED)
target_link_libraries(main PRIVATE wincpp::wincpp wincpp::_wincpp_core)
Is wincpp::_wincpp_core intended to be exposed like this? If not we need a usage file.
| } | ||
| case wincpp::memory_type::remote_t: | ||
| { | ||
| - std::size_t read; |
There was a problem hiding this comment.
Can you explain what this is doing and/or submit it upstream?
| @@ -0,0 +1,22 @@ | |||
| vcpkg_from_github( | |||
There was a problem hiding this comment.
Please change this file to LF line endings.
| + | ||
| +include(CMakePackageConfigHelpers) | ||
| +write_basic_package_version_file( | ||
| + "${CMAKE_CURRENT_BINARY_DIR}/wincppConfigVersion.cmake" |
There was a problem hiding this comment.
Do we need a vcpkg_cmake_config_fixup call for this?
| # Add the library to the project | ||
| -add_library(wincpp STATIC) | ||
| +add_library(wincpp) | ||
| +set_target_properties(wincpp PROPERTIES WINDOWS_EXPORT_ALL_SYMBOLS ON) |
There was a problem hiding this comment.
Note that you can use vcpkg_check_linkage(ONLY_STATIC_LIBRARY)
|
I checked the optional dependencies line rather than crossing it out because it looks like that is done here. |
3 participants
vcpkg.json, or explicitly disabled through patches or build system arguments such as CMAKE_DISABLE_FIND_PACKAGE_Xxx or VCPKG_LOCK_FIND_PACKAGEvcpkg.jsonmatches what upstream says.vcpkg.jsonmatches what upstream says../vcpkg x-add-version --alland committing the result.